Create schema submodule #248

elezar · 2025-02-04T20:15:53Z

The github.com/xeipuuv/gojsonschema that is imported for schema validation can be problematic.

This change creates a go submodule at schema to remove this dependency from the main module.

Note that this also moves from a function-based schema validation to using an interface so that the schema-based validators are pluggable into the producer as implemented in #233

bart0sh · 2025-02-05T09:21:32Z

pkg/cdi/cache_test.go

 func TestRefreshCache(t *testing.T) {
+	if runtime.GOOS == "darwin" {
+		t.Skip("skipping on darwin")
+	}


Can you explain why we're skipping this test on Darwin?

I didn't look into it too deeply, but my assumption was that the ordering is different there causing the refreshed state to not be the same as encoded in the tests.

I can update the message to make this clearer if you prefer. (to be clear this is actually part of #249)

bart0sh · 2025-02-05T09:34:50Z

@elezar schema unit tests fail to build it seems:

Running tool: /usr/local/go/bin/go test -timeout 300s -coverprofile=/tmp/vscode-goYWugaU/go-code-cover tags.cncf.io/container-device-interface/schema

# tags.cncf.io/container-device-interface/schema_test [tags.cncf.io/container-device-interface/schema.test]
/home/ed/git/container-device-interface/schema/schema_test.go:301:22: cannot use bytes.NewReader(data) (value of type *bytes.Reader) as *"tags.cncf.io/container-device-interface/specs-go".Spec value in argument to scm.Validate
/home/ed/git/container-device-interface/schema/schema_test.go:303:16: undefined: schema.Validate
/home/ed/git/container-device-interface/schema/schema_test.go:318:22: cannot use r (variable of type io.Reader) as *"tags.cncf.io/container-device-interface/specs-go".Spec value in argument to scm.Validate
/home/ed/git/container-device-interface/schema/schema_test.go:320:16: undefined: schema.Validate
/home/ed/git/container-device-interface/schema/schema_test.go:326:22: cannot use bytes.NewReader(buf.Bytes()) (value of type *bytes.Reader) as *"tags.cncf.io/container-device-interface/specs-go".Spec value in argument to scm.Validate
/home/ed/git/container-device-interface/schema/schema_test.go:328:16: undefined: schema.Validate
FAIL	tags.cncf.io/container-device-interface/schema [build failed]
FAIL

elezar · 2025-02-05T10:29:15Z

Thanks @bart0sh.

I wrongly assumed that the make test target was also triggering the unit tests in the schema, but it doesn't handle submodules. I have updated the make target to also work with submoldules so that the unit tests there are also triggered and fixed them.

bart0sh · 2025-02-05T11:19:44Z

@elezar There is something wrong with the package dependencies, I guess. go list doesn't show all of them. This could be a reason why go test ./... doesn't run all tests.

$ go list ./...
tags.cncf.io/container-device-interface/internal/validation
tags.cncf.io/container-device-interface/internal/validation/k8s
tags.cncf.io/container-device-interface/pkg/cdi
tags.cncf.io/container-device-interface/pkg/parser

bart0sh · 2025-02-05T11:20:25Z

why schema_test.go uses its own package schema_test ?

bart0sh · 2025-02-05T12:03:09Z

Makefile

+	$(Q)for mod in $$(find . -name go.mod); do \
+	    echo "Testing $$(dirname $$mod)..."; ( \
+	        cd $$(dirname $$mod) && $(GO_TEST) ./... \
+            ) || exit 1; \


Would it exit on the first test failure? If so, could it be modified to run all tests even if one fails?

I have updated this to run all tests and exit on failures afterwards.

elezar · 2025-02-05T13:51:06Z

why schema_test.go uses its own package schema_test ?

I didn't change the test package as part of this change. Maybe the original intent was to test against public interfaces. @klihub do you recall if there was a reason for this?

A few thinks I missed need to be cleared.

elezar · 2025-02-05T14:10:42Z

@elezar There is something wrong with the package dependencies, I guess. go list doesn't show all of them. This could be a reason why go test ./... doesn't run all tests.
$ go list ./...
tags.cncf.io/container-device-interface/internal/validation
tags.cncf.io/container-device-interface/internal/validation/k8s
tags.cncf.io/container-device-interface/pkg/cdi
tags.cncf.io/container-device-interface/pkg/parser

Go list lists packages in the current module. When you're in the root folder the packages that you list above are the only packages. We already have a number of submodules:

$ find . -name go.mod
./cmd/validate/go.mod
./cmd/cdi/go.mod
./go.mod
./specs-go/go.mod
./schema/go.mod

where schema was added in thie PR.

klihub · 2025-02-05T15:04:00Z

why schema_test.go uses its own package schema_test ?

I didn't change the test package as part of this change. Maybe the original intent was to test against public interfaces. @klihub do you recall if there was a reason for this?

Yes, we do blackbox testing using the public interfaces only. It's normal go practice to put the tests in ${package}_test in that case.

pkg/cdi/cache.go

bart0sh · 2025-02-05T15:55:31Z

Yes, we do blackbox testing using the public interfaces only. It's normal go practice to put the tests in ${package}_test in that case.

Is this package somehow different from others in this project?

$ find . -name *_test.go |xargs grep package
./pkg/parser/parser_test.go:package parser
./pkg/cdi/default-cache_test.go:package cdi
./pkg/cdi/regressions_test.go:package cdi
./pkg/cdi/annotations_test.go:package cdi
./pkg/cdi/container-edits_test.go:package cdi
./pkg/cdi/spec-dirs_test.go:package cdi
./pkg/cdi/device_test.go:package cdi
./pkg/cdi/cache_test.go:package cdi
./pkg/cdi/spec_test.go:package cdi
./schema/schema_test.go:package schema_test

klihub · 2025-02-06T09:15:33Z

Yes, we do blackbox testing using the public interfaces only. It's normal go practice to put the tests in ${package}_test in that case.

Is this package somehow different from others in this project?

$ find . -name *_test.go |xargs grep package ./pkg/parser/parser_test.go:package parser ./pkg/cdi/default-cache_test.go:package cdi ./pkg/cdi/regressions_test.go:package cdi ./pkg/cdi/annotations_test.go:package cdi ./pkg/cdi/container-edits_test.go:package cdi ./pkg/cdi/spec-dirs_test.go:package cdi ./pkg/cdi/device_test.go:package cdi ./pkg/cdi/cache_test.go:package cdi ./pkg/cdi/spec_test.go:package cdi ./schema/schema_test.go:package schema_test

I think many of the tests in pkg/cdi make minimal use of some unexported functions (for instance newCache, newSpec, validation functions, those sort of things) and that's why they were made part of the package they test. The schema package is mostly a pile of boilerplate with the specific purpose to hide and isolate the xeipuuv/gojsonschema interfaces, transforming them to something behind which we could plug in another implementation without changes to the rest of the code if/once we found one more adequate package. So there it was an obvious choice to go for blackbox testing.

bart0sh · 2025-02-06T11:36:34Z

@klihub Thank you for the explanation! Now that makes sense to me. parser tests can also use its own package it seems.

elezar · 2025-02-06T11:45:15Z

@klihub Thank you for the explanation! Now that makes sense to me. parser tests can also use its own package it seems.

Sure, but that is out of scope for this PR. I'm happy to address that as a follow-up.

Signed-off-by: Evan Lezar <[email protected]>

This creates a schema submodule to remove dependencies from the cdi package that is more commonly used. Signed-off-by: Evan Lezar <[email protected]>

Signed-off-by: Evan Lezar <[email protected]>

bart0sh · 2025-02-06T13:29:36Z

/lgtm

elezar force-pushed the schema-submodule branch 4 times, most recently from 21a575e to b05721a Compare February 4, 2025 20:51

elezar changed the title ~~Schema submodule~~ Create schema submodule Feb 4, 2025

elezar marked this pull request as ready for review February 4, 2025 20:53

elezar requested review from bart0sh and klihub February 4, 2025 20:54

klihub previously approved these changes Feb 5, 2025

View reviewed changes

klihub mentioned this pull request Feb 5, 2025

Fix cache refresh on darwin #249

Merged

bart0sh reviewed Feb 5, 2025

View reviewed changes

elezar force-pushed the schema-submodule branch from b05721a to f37288d Compare February 5, 2025 09:22

elezar force-pushed the schema-submodule branch from f37288d to 8070930 Compare February 5, 2025 10:27

elezar requested a review from bart0sh February 5, 2025 10:29

bart0sh reviewed Feb 5, 2025

View reviewed changes

klihub self-requested a review February 5, 2025 13:55

elezar force-pushed the schema-submodule branch from 8070930 to f1f5c60 Compare February 5, 2025 13:59

elezar force-pushed the schema-submodule branch from f1f5c60 to 991563e Compare February 5, 2025 14:15

klihub reviewed Feb 5, 2025

View reviewed changes

pkg/cdi/cache.go Outdated Show resolved Hide resolved

elezar added 3 commits February 6, 2025 12:47

Check for no devices in spec validation

df3f1f3

Signed-off-by: Evan Lezar <[email protected]>

Move schema to a separate go module

53c17be

This creates a schema submodule to remove dependencies from the cdi package that is more commonly used. Signed-off-by: Evan Lezar <[email protected]>

Use validator interface instead of function

af0ba04

Signed-off-by: Evan Lezar <[email protected]>

elezar force-pushed the schema-submodule branch from 991563e to af0ba04 Compare February 6, 2025 11:47

bart0sh approved these changes Feb 6, 2025

View reviewed changes

klihub self-requested a review February 6, 2025 13:44

klihub approved these changes Feb 6, 2025

View reviewed changes

klihub merged commit 857ddea into cncf-tags:main Feb 6, 2025
8 checks passed

elezar deleted the schema-submodule branch February 17, 2025 14:03

Create schema submodule #248

Create schema submodule #248

Uh oh!

Conversation

elezar commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bart0sh Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

elezar Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

bart0sh commented Feb 5, 2025

Uh oh!

elezar commented Feb 5, 2025

Uh oh!

bart0sh commented Feb 5, 2025

Uh oh!

bart0sh commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bart0sh Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

elezar Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

elezar commented Feb 5, 2025

Uh oh!

elezar commented Feb 5, 2025

Uh oh!

klihub commented Feb 5, 2025

Uh oh!

Uh oh!

bart0sh commented Feb 5, 2025

Uh oh!

klihub commented Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bart0sh commented Feb 6, 2025

Uh oh!

elezar commented Feb 6, 2025

Uh oh!

bart0sh commented Feb 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elezar commented Feb 4, 2025 •

edited

Loading

bart0sh commented Feb 5, 2025 •

edited

Loading

klihub commented Feb 6, 2025 •

edited

Loading